CountDownLatch Synchronization for RCPTestWorkbenchAdvisor #3429

vogella · 2025-10-22T15:20:42Z

The flaky test was caused by a race condition in
RCPTestWorkbenchAdvisor.java. The test spawns 4 background threads during preStartup() that schedule async/sync runnables on the display.

However, there was no guarantee these runnables would execute before postStartup() marked the workbench as "started" (by setting started = true).

This meant asyncWithDisplayAccess could still be null when the test assertions ran, causing intermittent failures.

This change adds a CountDownLatch to ensure all 4 async/sync operations complete before marking the workbench as started:

Key changes to
/tests/org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java:

Added latch field (line 48):
- private static java.util.concurrent.CountDownLatch asyncLatch = null;
Initialize latch in preStartup() (line 130):
- asyncLatch = new java.util.concurrent.CountDownLatch(4); — tracks 4 operations
Count down after each operation completes:
- In setupSyncDisplayThread(): Added finally block (lines 179-183) that calls asyncLatch.countDown()
- In setupAsyncDisplayThread(): Added finally block (lines 205-209) that calls asyncLatch.countDown()
Wait for operations in postStartup() (lines 222-235):
- Waits up to 5 seconds for all operations to complete
- Logs warnings if timeout occurs or thread is interrupted
- Only marks started = true after all operations complete

Fixes #1517

eclipse-platform-bot · 2025-10-22T15:27:25Z

This pull request changes some projects for the first time in this development cycle.
Therefore the following files need a version increment:

tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF

An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch.

Git patch

From 8bf477c3eba2a22ebaac6bf63bdf0487305134f7 Mon Sep 17 00:00:00 2001
From: Eclipse Platform Bot <[email protected]>
Date: Wed, 22 Oct 2025 15:26:58 +0000
Subject: [PATCH] Version bump(s) for 4.38 stream


diff --git a/tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF b/tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF
index 259fee3e99..8cdb2f2c5f 100644
--- a/tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF
+++ b/tests/org.eclipse.ui.tests.harness/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Manifest-Version: 1.0
 Bundle-ManifestVersion: 2
 Bundle-Name: Harness Plug-in
 Bundle-SymbolicName: org.eclipse.ui.tests.harness;singleton:=true
-Bundle-Version: 1.10.600.qualifier
+Bundle-Version: 1.10.700.qualifier
 Eclipse-BundleShape: dir
 Require-Bundle: org.eclipse.ui,
  org.eclipse.core.runtime,
-- 
2.51.0

Further information are available in Common Build Issues - Missing version increments.

github-actions · 2025-10-22T16:32:50Z

Test Results

3 018 files ±0 3 018 suites ±0 2h 38m 2s ⏱️ + 7m 48s
8 234 tests ±0 7 960 ✅ - 24 249 💤 ±0 2 ❌ +1 23 🔥 +23
23 622 runs ±0 22 755 ✅ - 72 794 💤 ±0 4 ❌ +3 69 🔥 +69

For more details on these failures and errors, see this check.

Results for commit 13a3215. ± Comparison against base commit 1a4d1a8.

♻️ This comment has been updated with latest results.

....eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java

fedejeanne · 2025-10-22T18:41:30Z

....eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java

+				if (!completed) {
+					System.err.println("Warning: Not all async/sync operations completed within timeout");
+				}


I would let it fail. I'd rather have a test that fails consistently than one that might fail even if completed == false.

Suggested change

if (!completed) {

System.err.println("Warning: Not all async/sync operations completed within timeout");

}

assertTrue(completed, "Not all async/sync operations completed within timeout");

Hm, it seems I missed an important part: when this fails, the workbench is already created and therefore subsequent attempts to create it will fail too.

After the latest changes, the logs show that once this happens, a bunch of tests fail with the error Workbench already exists and cannot be created again

Here are the first 3 failures.

Stack traces

org.eclipse.ui.tests.rcp.PlatformUITest.testCreateAndRunWorkbench -- Time elapsed: 7.070 s <<< FAILURE! java.lang.AssertionError: Not all async/sync operations completed within timeout at org.eclipse.ui.tests.harness.util.RCPTestWorkbenchAdvisor.postStartup(RCPTestWorkbenchAdvisor.java:221) at org.eclipse.ui.tests.rcp.util.WorkbenchAdvisorObserver.postStartup(WorkbenchAdvisorObserver.java:140) at org.eclipse.ui.internal.Workbench.lambda$18(Workbench.java:2901) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1104) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1038) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:153) at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:677) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:583) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173) at org.eclipse.ui.tests.rcp.PlatformUITest.testCreateAndRunWorkbench(PlatformUITest.java:57) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) org.eclipse.ui.tests.rcp.PlatformUITest.testCreateAndRunWorkbenchWithExceptionOnStartup skipped Running RcpTestSuite WorkbenchAdvisorTest Tests run: 9, Failures: 0, Errors: 9, Skipped: 0, Time elapsed: 0.428 s <<< FAILURE! -- in RcpTestSuite WorkbenchAdvisorTest org.eclipse.ui.tests.rcp.WorkbenchAdvisorTest.testEmptyProgressRegion -- Time elapsed: 0.191 s <<< ERROR! java.lang.IllegalStateException: Workbench already exists and cannot be created again. at org.eclipse.ui.internal.Workbench.<init>(Workbench.java:481) at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:609) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:583) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173) at org.eclipse.ui.tests.rcp.WorkbenchAdvisorTest.testEmptyProgressRegion(WorkbenchAdvisorTest.java:293) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) org.eclipse.ui.tests.rcp.WorkbenchAdvisorTest.testFillAllActionBar -- Time elapsed: 0.031 s <<< ERROR! java.lang.IllegalStateException: Workbench already exists and cannot be created again. at org.eclipse.ui.internal.Workbench.<init>(Workbench.java:481) at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:609) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:583) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173) at org.eclipse.ui.tests.rcp.WorkbenchAdvisorTest.testFillAllActionBar(WorkbenchAdvisorTest.java:279) at java.base/java.lang.reflect.Method.invoke(Method.java:580) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596) at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

The 1st failure is due to the timeout (Not all async/sync operations completed within timeout) and the workbench is not cleaned-up/shutdown. The following failing tests also fail with Workbench already exists and cannot be created again..

The way I see it, we need 2 things:

See why the timeout occurs (is it "just" a matter of waiting a bit longer since the hardware is old and slow or is it something else?)

Clean-up if the timeout occurs so that the following tests may start the workbench again.

I don't have any novel ideas at the moment :-\ and you @vogella ?

HeikoKlare

The change looks sound. Thank you for taking care of that long-standing failing test. I just have minor comments regarding a potential improvement/simplification.

HeikoKlare · 2025-10-28T09:11:42Z

....eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java

+		Display display = Display.getCurrent();
+		if (display != null) {
+			// Process pending async events multiple times to ensure they execute
+			for (int i = 0; i < 10; i++) {
+				while (display.readAndDispatch()) {
+					// Keep processing events
+				}
+				// Small yield to allow any final async operations to complete
+				try {
+					Thread.sleep(10);
+				} catch (InterruptedException e) {
+					Thread.currentThread().interrupt();
+					break;
+				}
+			}
+		}


Is this quite complex event processing really necessary? All relevant runnables are guaranteed to be scheduled here, so display.readAndDispatch() should not yield false until they all have been processed. You just need to make sure that those runnables that are not supposed to be executed during startup had the chance to accidentally be executed (to detect regression). So wouldn't be something like this be sufficient?

Suggested change

Display display = Display.getCurrent();

if (display != null) {

// Process pending async events multiple times to ensure they execute

for (int i = 0; i < 10; i++) {

while (display.readAndDispatch()) {

// Keep processing events

}

// Small yield to allow any final async operations to complete

try {

Thread.sleep(10);

} catch (InterruptedException e) {

Thread.currentThread().interrupt();

break;

}

}

}

UITestUtil.processEventsUntil(() -> syncWithDisplayAccess && asyncWithDisplayAccess, 5000);

UITestUtil.processEvents();

HeikoKlare · 2025-10-28T09:18:11Z

....eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util/RCPTestWorkbenchAdvisor.java

 	private static boolean started = false;

+	// Use a CountDownLatch to ensure async operations complete before postStartup
+	private static final CountDownLatch asyncLatch = new CountDownLatch(4);


nit: wouldn't it be cleaner to use something like a Phaser and make every thread register itself and then deregister/arrive when being executed (like now done with asyncLatch.countDown()), so that the latch's count does not need to magically match the number of thread created?

…tion Replace CountDownLatch with Phaser to eliminate magic number and simplify event processing using UITestUtil helpers per review feedback. - Use Phaser instead of CountDownLatch for cleaner thread tracking - Simplify postStartup() event loop with UITestUtil.processEventsUntil() - Each thread self-registers with phaser, removing hardcoded count 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

vogella mentioned this pull request Oct 22, 2025

Migrate JUnit 4 tests to JUnit 5 in org.eclipse.jface.text.tests #3410

Closed

fedejeanne reviewed Oct 22, 2025

View reviewed changes

vogella force-pushed the flacky-test-RCP-test-advisor branch 3 times, most recently from bebe15f to cd9113d Compare October 23, 2025 14:38

HeikoKlare reviewed Oct 28, 2025

View reviewed changes

vogella force-pushed the flacky-test-RCP-test-advisor branch from cd9113d to 13a3215 Compare November 3, 2025 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CountDownLatch Synchronization for RCPTestWorkbenchAdvisor #3429

CountDownLatch Synchronization for RCPTestWorkbenchAdvisor #3429

vogella commented Oct 22, 2025

Uh oh!

eclipse-platform-bot commented Oct 22, 2025

Uh oh!

github-actions bot commented Oct 22, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fedejeanne Oct 22, 2025

Uh oh!

fedejeanne Oct 23, 2025

Uh oh!

HeikoKlare left a comment

Uh oh!

HeikoKlare Oct 28, 2025

Uh oh!

HeikoKlare Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CountDownLatch Synchronization for RCPTestWorkbenchAdvisor #3429

Are you sure you want to change the base?

CountDownLatch Synchronization for RCPTestWorkbenchAdvisor #3429

Conversation

vogella commented Oct 22, 2025

Uh oh!

eclipse-platform-bot commented Oct 22, 2025

Uh oh!

github-actions bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fedejeanne Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

fedejeanne Oct 23, 2025

Choose a reason for hiding this comment

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

HeikoKlare Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

HeikoKlare Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Oct 22, 2025 •

edited

Loading